Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Disable HW acceleration on Aztec when it crashes on Android 8 - Release Gutenberg to v1.15.1 #10620

Merged
merged 14 commits into from
Oct 22, 2019
Merged

Disable HW acceleration on Aztec when it crashes on Android 8 - Release Gutenberg to v1.15.1 #10620

merged 14 commits into from
Oct 22, 2019

Conversation

daniloercoli
Copy link
Contributor

@daniloercoli daniloercoli commented Oct 17, 2019 •
edited
Loading

This PR tries to mitigate #8828 by catching the crash reported by Aztec, and disabling HW acceleration on the current post. (The postID is stored in AppPrefs. Next time the users load the same post, the app turns HW acceleration off for the whole Aztec Fragment).

The original problem, as discussed with details in #8828, is deep inside the Android 8 Framework, and there is no way for us to prevent it. We found out that disabling HW acc. does fix it.
Unfortunately we cannot disable HW acc. on all posts, since without HW acc. the Aztec editor is kind of slow even on recent devices. More, the problem doesn't happen all the times, but only when the users follow some edit patterns (we cannot repro it btw) that make it happen.

In details:

  • Aztec was modified to report the out of bounds problem android.text.DynamicLayout.getBlockIndex(DynamicLayout.java:646)
  • The app catches this exact error, and writes in AppPrefs the id of the post
  • When the user will load the same post again, Aztec is started without HW acc.

Note: Unfortunately this PR does act after the fact, and probably we won't see crash report numbers disappear completely.

As a further improvement to this, we may want to add (?) a global app setting, say "Aztec compatibility mode" that we switch to ON the first this crash happens, and notify the user about that, and give the instructions on how to turn it OFF in App->Settings.

cc @designsimply

This PR also updates Gutenberg to v1.15.1
gutenberg-mobile PR: wordpress-mobile/gutenberg-mobile#1479

TODO:

  • Update gutenberg ref to point at 1.15.1 tag

PR submission checklist:

  • [ x ] I have considered adding unit tests where possible.

  • [ x ] I have considered if this change warrants user-facing release notes and have added them to RELEASE-NOTES.txt if necessary.

@daniloercoli daniloercoli requested a review from mzorz October 17, 2019 10:02
@daniloercoli daniloercoli self-assigned this Oct 17, 2019
@daniloercoli daniloercoli modified the milestones: 13.5, 13.6 Oct 17, 2019
@peril-wordpress-mobile
Copy link

peril-wordpress-mobile bot commented Oct 17, 2019 •
edited
Loading

You can test the changes on this Pull Request by downloading the APK here.

@mzorz mzorz self-assigned this Oct 17, 2019
@mzorz mzorz mentioned this pull request Oct 21, 2019
Closed
8 tasks
mzorz
mzorz reviewed Oct 21, 2019
View reviewed changes
Copy link
Contributor

@mzorz mzorz left a comment •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Managed to reproduce the crash by

  1. setting visibility=gone to the editor title,
  2. removing the crash preventer InputFilter in AztecText (in setupKeyListenersAndInputFilters()),
  3. make sure the site is not private (so crash logging is enabled)
  4. adding a captioned image to a draft post
  5. the image needs to be large, like, occupying more than it fits the view (for example, even a screenshot of the device which is tall enough to exceed the editor window)
  6. open the post and scroll the image a bit, so you can see the middle of the image but can’t see where the cursor is, nor your characters
  7. then trying to insert something at the beginning (as per the crash described in aztec repo)

With this I was able to test and found some problems, left comments there in the PR (https://github.com/wordpress-mobile/WordPress-Android/pull/10620/files#r337038375 and https://github.com/wordpress-mobile/WordPress-Android/pull/10620/files#r337053623), I think once these problems are fixed we should be good to go 👍

@mzorz
Copy link
Contributor

mzorz commented Oct 21, 2019

Just a note on my steps to reproduce above:

  1. make sure the site is not private (so crash logging is enabled)

this means that given we are relying on the logger for this solution, and we only have logging enabled for public sites, private sites won’t have the solution enabled.

I think we can go with this for now, adding some Tracks information when we mark a Post for HW acceleration disabling (and when we open a post with HW acceleration disabled) and see how it goes, what do you think?

@daniloercoli
Merge branch 'develop' of https://github.com/wordpress-mobile/WordPre…
0a3ee14 
…ss-Android into issue/8828-Aztec-crash-Android-8

* 'develop' of https://github.com/wordpress-mobile/WordPress-Android: (29 commits)
  Make utils sinletons
  Handle case without a browser
  FluxC hash updated to 1.5.1-beta-3
  Add signup flow name parameter to signup auth email request
  Change FluxC hash to 1.5.1-beta-2
  Update gutenberg-mobile ref to v1.15.0 release
  CircleCI: Bump Orbs to use any 1.0.x version
  Update gutenberg ref
  Update gutenberg ref
  Add NEW_TASK flag to the open file intent
  Update to Aztec v1.3.33, including gutenberg's ref
  Update gutenberg ref
  Update Aztec ref to v1.3.32
  Update RELEASE-NOTES with block editor improvements
  Enable downloads from private sites
  Update gutenberg ref to release 1.15.0
  capitalizeWithLocaleWithoutLint string extension introduced
  Implement specific comparison for LocalOrRemoteId in PostListDiffItemCallback
  Fix staticFieldLeak errors in lint-baseline.xml
  Refresh data on request completed
  ...

# Conflicts:
#	libs/editor/WordPressEditor/build.gradle
#	libs/gutenberg-mobile
@daniloercoli
Update GB hash and Aztec hash
af2e9b2
@daniloercoli
Bump analytics about HW Acc. status of the current loaded (in the edi…
54067d8 
…tor) post.
@daniloercoli
Copy link
Contributor Author

can go with this for now, adding some Tracks information when we mark a Post for HW acceleration disabling (and when we open a post with HW acceleration disabled) and see how it goes, what do you think?

I've added the HW Acc. disabled flag to Tracks, as a property of EDITOR_OPENED, and EDITOR_SESSION_* events.
I think those are fine for now, since the tracking of crashes already gives us the number of people that disabled HW Acc.

If that's ok for you we can merge this 3 PRs tomorrow!

@daniloercoli daniloercoli mentioned this pull request Oct 21, 2019
Closed
mzorz
mzorz approved these changes Oct 22, 2019
View reviewed changes
Copy link
Contributor

@mzorz mzorz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM :shipit:

@daniloercoli daniloercoli mentioned this pull request Oct 22, 2019
Merged
@daniloercoli daniloercoli mentioned this pull request Oct 22, 2019
Merged
4 tasks
@daniloercoli daniloercoli changed the title Disable HW acceleration on Aztec when it crashes on Android 8 Disable HW acceleration on Aztec when it crashes on Android 8 - Release Gutenberg to v1.15.1 Oct 22, 2019
@mchowning mchowning merged commit fec059a into develop Oct 22, 2019
@mchowning mchowning deleted the issue/8828-Aztec-crash-Android-8 branch October 22, 2019 18:10
@designsimply designsimply mentioned this pull request Nov 6, 2019
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mzorz mzorz mzorz approved these changes

Assignees

@daniloercoli daniloercoli

@mzorz mzorz

Labels
Aztec
Projects
None yet
Milestone
13.6
Development

Successfully merging this pull request may close these issues.
3 participants
@daniloercoli @mzorz @mchowning